Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support repeated query params and simplified "csv" cases #304

Merged
merged 2 commits into from
Sep 17, 2020
Merged

feat: support repeated query params and simplified "csv" cases #304

merged 2 commits into from
Sep 17, 2020
+84 −49

Conversation

zaquestion
Copy link
Member

@zaquestion zaquestion commented Aug 20, 2020 •
edited
Loading

This allows for leveraging repeated fields in the query via foo=bar&foo=bar2, this also simplifies the "csv" case for repeated strings. Before this using repeated string foo in the query would require quoted values foo="bar","bar2", which is a little weird as it departs from how we normally pass values in query params, so as special case for strings (and a more efficient one), foo=bar,bar2 is supported

Example generation using the protos from the 2-repeated integration test
String (note: zero json marshaling required for these)

	if MGetRepeatedStrArr, ok := queryParams["M"]; ok {
		MGetRepeatedStr := MGetRepeatedStrArr[0]

		var MGetRepeated []string
		if len(MGetRepeatedStrArr) > 1 {
			MGetRepeated = MGetRepeatedStrArr
		} else {
			MGetRepeated = strings.Split(MGetRepeatedStr, ",")
		}
		req.M = MGetRepeated
	}

Non string

	if LGetRepeatedStrArr, ok := queryParams["L"]; ok {
		LGetRepeatedStr := LGetRepeatedStrArr[0]

		var LGetRepeated []bool
		if len(LGetRepeatedStrArr) > 1 {
			LGetRepeated = make([]bool, len(LGetRepeatedStrArr))
			for i, v := range LGetRepeatedStrArr {
				converted, err := strconv.ParseBool(v)
				if err != nil {
					return nil, errors.Wrapf(err, "couldn't decode LGetRepeated from %v", LGetRepeatedStr)
				}
				LGetRepeated[i] = converted
			}
		} else {
			err = json.Unmarshal([]byte(LGetRepeatedStr), &LGetRepeated)
			if err != nil {
				LGetRepeatedStr = "[" + LGetRepeatedStr + "]"
			}
			err = json.Unmarshal([]byte(LGetRepeatedStr), &LGetRepeated)
		}
		if err != nil {
			return nil, errors.Wrapf(err, "couldn't decode LGetRepeated from %v", LGetRepeatedStr)
		}
		req.L = LGetRepeated
	}

Note: the else case above represents the existing code gen, while the else case for string had been optimized and removes the need for ", technically an in compatibility

@zaquestion zaquestion force-pushed the repeated_query_param_improvements branch 3 times, most recently from 82cd714 to 2f6ab2f Compare August 20, 2020 19:29
@lelandbatey
Copy link
Member

This MR is a welcome change. I've never been super happy with the way we deal with encoding more complex structs into query parameters, so in general I very much welcome any improvement to the query handling.

For this MR in particular, my request is that we have a new test added showing support for this different behavior.

@zwiedmann-isp
Copy link
Contributor

This change will address #238 as well.

@zaquestion zaquestion force-pushed the repeated_query_param_improvements branch from 2f6ab2f to db19ea7 Compare August 22, 2020 07:59
@zaquestion zaquestion mentioned this pull request Aug 26, 2020
Draft
@adamryman
Copy link
Member

Could you uncomment this line in the TestGetWithRepeatedQueryRequest integration test and see if it passes?

truss/cmd/_integration-tests/transport/http_test.go

Line 112 in db19ea7

//err := testHTTP(nil, "GET", "getwithrepeatedquery?%s=%d&%s=%d]", "A", A[0], "A", A[1])

I believe it should test the feature you are adding. Though I'm not 100% sure.

It it does not test your feature, could you drop a test in the linked file?

@adamryman adamryman mentioned this pull request Aug 31, 2020
Merged
@adamryman
Copy link
Member

Excited to see how everything looks once this is rebased on master.

@zaquestion
feat: support repeated query params and simplified "csv" cases
39871ea 
This allows for leveraging repeated fields in the query via foo=bar&foo=bar2, this also simplifies the "csv" case for repeated strings. Before this using `repeated string foo` in the query would require quoted values `foo="bar","bar2"`, which is a little weird as it departs from how we normally pass values in query params, so as special case for strings (and a more efficient one), `foo=bar,bar2` is supported
@zaquestion
update client tmpl to use new repeated query param strategy
5891a72 
expose tests added in #309
@zaquestion zaquestion force-pushed the repeated_query_param_improvements branch from db19ea7 to 5891a72 Compare September 6, 2020 01:30
@zaquestion
Copy link
Member Author

@adamryman PTAL, I've brought the latest changes with #309 and uncommented the tests. You'll notice that I removed a few cases as these changes do cause a few incompatibilities. Namely quoted array and quoted csv style no longer work for repeated strings. IMO having to do this before was so broken I'm content to call it a bug fix. If not, we can move these changes onto a v1 branch and I can go from there.

@zaquestion
Copy link
Member Author

Oh and good call about the clients, tests immediately caught the break there, I've updated the clients to use the new strategies when possible and both causes are already covered in the tests.

@zaquestion zaquestion force-pushed the repeated_query_param_improvements branch 2 times, most recently from db19ea7 to 5891a72 Compare September 6, 2020 02:25
@zaquestion zaquestion merged commit 60897aa into master Sep 17, 2020
@adamryman
Copy link
Member

I know this is already reviewed, though great job supporting the backwards compatibility!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lelandbatey lelandbatey lelandbatey approved these changes

@adamryman adamryman Awaiting requested review from adamryman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zaquestion @lelandbatey @zwiedmann-isp @adamryman